-
Notifications
You must be signed in to change notification settings - Fork 286
Reset self.sock if connection fails #298
base: develop
Are you sure you want to change the base?
Conversation
If we don't reset `self.sock` on connection error, then `my_socket.is_open()` will still return `True` after `TTransportException` is thrown.
Tests are failing, but only on Python 2.6. One of the failures seem to be |
An additional surprise. After attempting to close a failing socket, it's still reported as open: >>> from thriftpy.transport import TSocket
>>> t = TSocket(host='google.com', port=7777)
>>> t.open()
Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 96, in open
self.sock.connect(addr)
ConnectionRefusedError: [Errno 111] Connection refused
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3/dist-packages/thriftpy/transport/socket.py", line 104, in open
message="Could not connect to %s" % str(addr))
thriftpy.transport.TTransportException: TTransportException(type=1, message="Could not connect to ('google.com', 7777)")
>>> t.close()
>>> t.is_open()
True |
Otherwise, `self.is_open()` may still return `True` after `close()`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these changes make a lot of sense and might help solve some "mysterious" happybase (thriftpy user) connection related errors.
What is needed to get this in? Should I convert the examples above to unit tests and add them to https://github.com/eleme/thriftpy/blob/develop/tests/test_socket.py? EDIT: tests committed |
the travis build failed on py26, but i am not sure it has anything to do with these changes:
|
Maybe the |
didn't look closely, but the main reason is that python 2.6 does not have it:
|
Ok, the memoryview problem pops up because setup.py requires tornado version >=4.0 and < 5.0. tornado dropped support for Python 2.6 in 4.4.0 (http://www.tornadoweb.org/en/stable/releases/v4.4.0.html) but pip installs tornado==4.5.1. Essentially, thriftpy no longer supports python 2.6 with the curent setup.py. Also, the Travis log has this: So, is it time for thriftpy to drop support for Python2.6? I'll happily do it here to make Travis happy, but I think it's better suited for a separate PR. |
Some (all?) pypy tests related to unix sockets are failing with |
If we don't reset
self.sock
on connection error, thenmy_socket.is_open()
will still returnTrue
afterTTransportException
is thrown. This may lead consumers to believe theTSocket
can still be used. Example:The code that lead to this was more complicated, of course, but the point is that without this patch, consumers need to be very careful with error handling.